Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Authentications Read and Delete api #13780

Merged
merged 2 commits into from
Feb 15, 2017
Merged

Authentications Read and Delete api #13780

merged 2 commits into from
Feb 15, 2017

Conversation

jntullo
Copy link

@jntullo jntullo commented Feb 6, 2017

This API is to read and delete Authentication resources.

Need correct product features for this PR (currently just has a placeholder).

Links

@miq-bot add_label wip, enhancement, api, euwe/no
@miq-bot assign @abellotti

@bdunne
Copy link
Member

bdunne commented Feb 7, 2017

Wouldn't it be better to set the endpoint to the higher-level concept (Authentications) and allow the requester to filter by type?

@jntullo
Copy link
Author

jntullo commented Feb 7, 2017

@bdunne I considered that, but that was out of the scope of my pivotal ticket so I thought I would narrow it. Also, create / update of these credentials (ie #13773) will work differently than some of the other Authentications so narrowing the scope resolves that issue as well. Happy to reconsider though.

thoughts @abellotti @gmcculloug @lgalis ?

@imtayadeway
Copy link
Contributor

Please rebase this PR to incorporate #13846 in order to ensure api.yml integrity ❤️ ❤️ ❤️

@jntullo jntullo changed the title [WIP] automation manager authentications read and delete api [WIP] Authentications Read and Delete api Feb 10, 2017
@miq-bot
Copy link
Member

miq-bot commented Feb 10, 2017

Checked commits jntullo/manageiq@58c837d~...f6d106e with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks good. 👍

:collection_actions:
:get:
- :name: read
:identifier: embedded_automation_manager_credentials_view
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abellotti how should we handle this identifier? We are using the Authentication model, but only have this identifier currently. Is it fine as is, and as needed we will add additional ones via #13827 ?

Copy link
Member

@abellotti abellotti Feb 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thought behind #13827 is that higher level collections are authorizable if the user has at least one of the related/relevant roles, so if a user has embedded_automation_manager_credentials_view or later automation_manager_credentials_view, etc. he'd be able to access this collection.

Signature above is fine, once #13827 is merged, you'd also use the array equivalent:

  :get:
  - :name: read
     :identifier:
     - embedded_automation_manager_credentials_view
     - automation_manager_credentials_view  (adding later whatever additional ones materialize as )

@jntullo jntullo changed the title [WIP] Authentications Read and Delete api Authentications Read and Delete api Feb 10, 2017
@jntullo
Copy link
Author

jntullo commented Feb 10, 2017

@miq-bot remove_label wip

@abellotti
Copy link
Member

Was a bit concerned that this was returning everything (was running as admin), but ran as a non-admin user without the embedded_automation_manager_credentials_view role, and got the standard 403. So, I'd 👍 with this.

@abellotti abellotti merged commit 55ab1e6 into ManageIQ:master Feb 15, 2017
@abellotti abellotti added this to the Sprint 55 Ending Feb 27, 2017 milestone Feb 15, 2017
@jntullo jntullo deleted the enhancement/credentials_api branch November 28, 2017 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants